Draft: Add input and volume support for BDV N9200W#84
Draft: Add input and volume support for BDV N9200W#84broglep wants to merge 7 commits intorytilahti:masterfrom
Conversation
broglep
force-pushed
the
feat/bdv_n9200w_support
branch
2 times, most recently
from
dd3bed7 to
17fa226
Compare
broglep
force-pushed
the
feat/bdv_n9200w_support
branch
from
17fa226 to
7b5114f
Compare
rytilahti
left a comment
rytilahti
left a comment
There was a problem hiding this comment.
Thanks for the PR! 👍 Here's just a very, very quick initial review.
songpal/device.py
Outdated
| except SongpalException as e: | ||
| found_services = None | ||
| if e.code == 12 and e.error_message == "getSupportedApiInfo": | ||
| found_services = await self._get_supported_methods_upnp() |
There was a problem hiding this comment.
Please separate all UPnP functionality into a separate class.
There was a problem hiding this comment.
Some UPnP code is also inside the containers (such as input & volume), how would you recommend separating this?
Having separate UPnP containers that subclass the regular container?
There was a problem hiding this comment.
Yeah, I think separate classes implementing the same interface is the way to go. Maybe something like VolumeControl, InputControl, which provide the common interface that gets implemented either by songpal or upnp impl, which gets chosen during the initialization procedures?
The code is quite tangled at the moment, especially on the containers and the device files, which was one of the reasons I started refactoring towards service-based classes.
songpal/device.py
Outdated
| return addr.lower().replace("-", ":") if addr else addr | ||
|
|
||
| macAddr = get_addr(info, "eth0") | ||
| wirelessMacAddr = get_addr(info, "wlan0") |
There was a problem hiding this comment.
Response from getNetworkSettings should be contained in its own container, NetworkSettings maybe?
Here's what the structure contains based on some logs I have:
DEBUG:songpal.method:system.getNetworkSettings outs: {'netif': <class 'str'>, 'hwAddr': <class 'str'>, 'ipAddrV4': <class 'str'>, 'ipAddrV6': <class 'str'>, 'netmask': <class 'str'>, 'gateway': <class 'str'>, 'dns': 'string*'}
songpal/device.py
Outdated
| ) | ||
|
|
||
| if self._upnp_renderer is None: | ||
| media_renderers = await DmrDevice.async_search(timeout=1) |
There was a problem hiding this comment.
One possibility to skip multiple discovery queries would be to modify the original Discover.discover() not to filter with the scalar web api ST, and do filtering during the discovery process.
Attempt to UPnP play the same way as the Songpal app is doing
This reverts commit 2999778.
|
@rytilahti I've made the requested refactorings. As I have no real songpal device available, I could not test if my changes did have an undesired impact onto the songpal functionality |
rytilahti
left a comment
rytilahti
left a comment
There was a problem hiding this comment.
Hi @broglep and sorry for the late response! I haven't been using my old HTXT3 for some while, so excuse me for the delay....
The PR looks fine from the surface, but I haven't tested it and I'm sort of afraid to merge this and prepare a new release without testing it. I'll try to find some time to test this with my now-broken soundbar to verify that the basic functionality still works prior merging this.
| return await self.services[service][method](params) | ||
|
|
||
|
|
||
| class UpnpDevice(Device): |
There was a problem hiding this comment.
Please separate this class into its own file.
There was a problem hiding this comment.
tried that, but gives some circular dependency issues. would require everything to be even more split up (device, device factory and upnp device) as the init.py loads device.py
I find it more concise to have everything in the device.py (as Upnp is just a specalization of device)
2 participants
First working version of input & volume support. Not the cleanest code, but does the job.
Is implemented so that it should work with potential other variations of devices that have UPnP, but comes with the downside of multiple SSDP discoveries
Addresses #29 and maybe #23